feat(processors): index .doc/.docx, .xls/.xlsx and .msg documents - #1265
feat(processors): index .doc/.docx, .xls/.xlsx and .msg documents#1265cbcoutinho wants to merge 5 commits into
Conversation
|
Claude finished @cbcoutinho's task in 3m 25s —— View job Review
This is round 4 of review on this PR; rounds 1–3 already covered the substantive findings (settings-driven timeout, rendition size cap, Independently verified, not just re-read:
Minor, non-blocking observations:
Test coverage is genuinely strong: real OLE2 containers via No blocking issues found. Nice diff — the per-format "read it the way that measured best" reasoning is well-argued and the docstrings carry real information (numbers, not vibes). |
d3163fb to
d4eeb57
Compare
…unit Review follow-up on #1265. The `msg.py` module docstring still claimed the processor was "read with extract-msg" and cited extract-msg's numbers as the reason, left over from before the switch to the olefile reader -- while `_msg_reader.py`'s own docstring told the real story and extract-msg is not even a dependency. It now describes what the module actually does and points at the reader for the rationale, keeping the measured comparison but attributing it correctly. Rename `_libreoffice.convert`'s `timeout` to `timeout_seconds`, matching `document_parse_timeout_seconds` and its siblings, and document why the cap is a parameter at all rather than the caller's own `fail_after`: expiry is translated here into `LibreOfficeError`, the single exception type both callers already handle (python:S7483). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1c68b00 to
f260d26
Compare
|
Round-1 findings addressed in
Left as-is, with reasons:
3374 unit tests pass, 5 office integration tests against real LibreOffice pass; ruff/ty green. |
…n size Round-2 review follow-up on #1265. The LibreOffice timeout was hardcoded at 120s with no override, unlike every comparable timeout here (`document_parse_timeout_seconds`, `docling_timeout`, `unstructured_timeout`). Adds `document_office_timeout_seconds` and threads it through registration for both processors that convert. A rendition reached the PDF engine directly rather than back through ProcessorRegistry, so it skipped the size cap an uploaded PDF has to pass. The source is already capped before download, but rendering is not size-preserving -- a modest .doc of dense vector figures can render far larger -- so the cap is now applied to the bytes actually about to be parsed, reusing `oversize_result_for_size` rather than restating the threshold. Also from the review: say why `extract_images=False` is forced for renditions; note in `_msg_reader` that only the plain-text body property is read, and what it would take to add the HTML/RTF variants; cover the legacy `.xls` branch with mocked unit tests so it is not exercised only by the LibreOffice-gated integration test; and cover the code-page decode path and its unknown-CPID fallback, which the `_build_msg` fixture could already produce but no test used. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…n size Round-2 review follow-up on #1265. The LibreOffice timeout was hardcoded at 120s with no override, unlike every comparable timeout here (`document_parse_timeout_seconds`, `docling_timeout`, `unstructured_timeout`). Adds `document_office_timeout_seconds` and threads it through registration for both processors that convert. A rendition reached the PDF engine directly rather than back through ProcessorRegistry, so it skipped the size cap an uploaded PDF has to pass. The source is already capped before download, but rendering is not size-preserving -- a modest .doc of dense vector figures can render far larger -- so the cap is now applied to the bytes actually about to be parsed, reusing `oversize_result_for_size` rather than restating the threshold. Also from the review: say why `extract_images=False` is forced for renditions; note in `_msg_reader` that only the plain-text body property is read, and what it would take to add the HTML/RTF variants; cover the legacy `.xls` branch with mocked unit tests so it is not exercised only by the LibreOffice-gated integration test; and cover the code-page decode path and its unknown-CPID fallback, which the `_build_msg` fixture could already produce but no test used. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
e2a58e9 to
be60182
Compare
|
Round-2 findings addressed in
Also documented the two-caps-and-a-slot interaction in 3391 unit tests pass, 5 office integration tests against real LibreOffice pass; ruff/ty green. |
…unit Review follow-up on #1265. The `msg.py` module docstring still claimed the processor was "read with extract-msg" and cited extract-msg's numbers as the reason, left over from before the switch to the olefile reader -- while `_msg_reader.py`'s own docstring told the real story and extract-msg is not even a dependency. It now describes what the module actually does and points at the reader for the rationale, keeping the measured comparison but attributing it correctly. Rename `_libreoffice.convert`'s `timeout` to `timeout_seconds`, matching `document_parse_timeout_seconds` and its siblings, and document why the cap is a parameter at all rather than the caller's own `fail_after`: expiry is translated here into `LibreOfficeError`, the single exception type both callers already handle (python:S7483). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…n size Round-2 review follow-up on #1265. The LibreOffice timeout was hardcoded at 120s with no override, unlike every comparable timeout here (`document_parse_timeout_seconds`, `docling_timeout`, `unstructured_timeout`). Adds `document_office_timeout_seconds` and threads it through registration for both processors that convert. A rendition reached the PDF engine directly rather than back through ProcessorRegistry, so it skipped the size cap an uploaded PDF has to pass. The source is already capped before download, but rendering is not size-preserving -- a modest .doc of dense vector figures can render far larger -- so the cap is now applied to the bytes actually about to be parsed, reusing `oversize_result_for_size` rather than restating the threshold. Also from the review: say why `extract_images=False` is forced for renditions; note in `_msg_reader` that only the plain-text body property is read, and what it would take to add the HTML/RTF variants; cover the legacy `.xls` branch with mocked unit tests so it is not exercised only by the LibreOffice-gated integration test; and cover the code-page decode path and its unknown-CPID fallback, which the `_build_msg` fixture could already produce but no test used. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
be60182 to
b1d8700
Compare
|
Round-3: no code changes — the two substantive items were the round-2 pair (settings-driven conversion timeout, rendition size cap), both already in On your three observations:
Rebased onto the updated lower PRs (#1263's helper move, #1264's scope fix). 3374 unit tests pass; office integration tests green against real LibreOffice. |
…unit Review follow-up on #1265. The `msg.py` module docstring still claimed the processor was "read with extract-msg" and cited extract-msg's numbers as the reason, left over from before the switch to the olefile reader -- while `_msg_reader.py`'s own docstring told the real story and extract-msg is not even a dependency. It now describes what the module actually does and points at the reader for the rationale, keeping the measured comparison but attributing it correctly. Rename `_libreoffice.convert`'s `timeout` to `timeout_seconds`, matching `document_parse_timeout_seconds` and its siblings, and document why the cap is a parameter at all rather than the caller's own `fail_after`: expiry is translated here into `LibreOfficeError`, the single exception type both callers already handle (python:S7483). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…n size Round-2 review follow-up on #1265. The LibreOffice timeout was hardcoded at 120s with no override, unlike every comparable timeout here (`document_parse_timeout_seconds`, `docling_timeout`, `unstructured_timeout`). Adds `document_office_timeout_seconds` and threads it through registration for both processors that convert. A rendition reached the PDF engine directly rather than back through ProcessorRegistry, so it skipped the size cap an uploaded PDF has to pass. The source is already capped before download, but rendering is not size-preserving -- a modest .doc of dense vector figures can render far larger -- so the cap is now applied to the bytes actually about to be parsed, reusing `oversize_result_for_size` rather than restating the threshold. Also from the review: say why `extract_images=False` is forced for renditions; note in `_msg_reader` that only the plain-text body property is read, and what it would take to add the HTML/RTF variants; cover the legacy `.xls` branch with mocked unit tests so it is not exercised only by the LibreOffice-gated integration test; and cover the code-page decode path and its unknown-CPID fallback, which the `_build_msg` fixture could already produce but no test used. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
b1d8700 to
8814e69
Compare
…unit Review follow-up on #1265. The `msg.py` module docstring still claimed the processor was "read with extract-msg" and cited extract-msg's numbers as the reason, left over from before the switch to the olefile reader -- while `_msg_reader.py`'s own docstring told the real story and extract-msg is not even a dependency. It now describes what the module actually does and points at the reader for the rationale, keeping the measured comparison but attributing it correctly. Rename `_libreoffice.convert`'s `timeout` to `timeout_seconds`, matching `document_parse_timeout_seconds` and its siblings, and document why the cap is a parameter at all rather than the caller's own `fail_after`: expiry is translated here into `LibreOfficeError`, the single exception type both callers already handle (python:S7483). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…n size Round-2 review follow-up on #1265. The LibreOffice timeout was hardcoded at 120s with no override, unlike every comparable timeout here (`document_parse_timeout_seconds`, `docling_timeout`, `unstructured_timeout`). Adds `document_office_timeout_seconds` and threads it through registration for both processors that convert. A rendition reached the PDF engine directly rather than back through ProcessorRegistry, so it skipped the size cap an uploaded PDF has to pass. The source is already capped before download, but rendering is not size-preserving -- a modest .doc of dense vector figures can render far larger -- so the cap is now applied to the bytes actually about to be parsed, reusing `oversize_result_for_size` rather than restating the threshold. Also from the review: say why `extract_images=False` is forced for renditions; note in `_msg_reader` that only the plain-text body property is read, and what it would take to add the HTML/RTF variants; cover the legacy `.xls` branch with mocked unit tests so it is not exercised only by the LibreOffice-gated integration test; and cover the code-page decode path and its unknown-CPID fallback, which the `_build_msg` fixture could already produce but no test used. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
8814e69 to
63392b0
Compare
Round-4 review follow-up on #1265. The setting was explicitly modelled on `document_parse_timeout_seconds`, but only the model got a `Validator(..., gte=1)`. A 0 or negative value therefore reached `anyio.fail_after` in `_libreoffice.convert` and expired every conversion the instant it started -- so the misconfiguration presented as "every .doc/.docx fails to parse", with nothing pointing at the setting that caused it, where its two siblings fail fast at startup and name themselves. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Adds three processors, each reading its format the way that measured best rather than forcing one route on all of them. Word documents (.doc/.docx) go through a LibreOffice PDF rendition parsed at the structured tier. Legacy .doc has no working pure-Python reader at all -- markitdown raises UnsupportedFormatException and docling-serve returns status=failure -- and for .docx the rendition is *more* faithful than a direct parse, not less: 98.3% token recall with 22 table rows against mammoth's 17, because mammoth drops a vertically-merged cell and shifts the rest of that row one column left, landing answers under the wrong heading. The rendition is a real PDF, so page numbers and highlight geometry come from the existing path instead of a second implementation. Hyperlink targets are lost in exchange. The tier is forced, not classified: the classifier scores a rendition on text quality and returns tier='fast', and the fast tier extracts ZERO tables from one, because rendered borders are vector line-art that only find_tables sees. Spreadsheets (.xls/.xlsx) are read cell-by-cell and deliberately never rendered. PDF rendering paginated a real workbook into 25 mixed-orientation pages and recalled only 63.8% of the tokens a direct read recovers -- 567 cells against 2013 -- losing the question numbering entirely, because LibreOffice honours the print layout. Legacy .xls converts container-to- container via xlsx, which changes the format without re-laying it out. Chunks carry `sheet_boundaries` (sheet + cell range) where a PDF carries pages; a spreadsheet has no geometry to highlight. Outlook messages (.msg) are read with a small olefile-based reader rather than `extract-msg`, which depends on `red-black-tree-mod` -- published as an sdist only, so it cannot satisfy the image's `uv sync --no-build` (no dependency's setup.py may execute at build time, docker:S8541). The reader is ~80 lines and recovers slightly more than extract-msg did on a real 1.3 MB thread (15,313 characters against 15,303) about seven times faster, while dropping nine transitive dependencies. Reading *both* string variants is the substance of it: markitdown emits 27 bytes for that message -- the literal "# Email Message\n\n## Content" -- because it looks for a body variant the message does not carry, while the same message holds a 14,996-byte body in the other. Headers are indexed as text, not just metadata: who sent it and when is much of what an inbox is searched for. Fields are read defensively, since .msg also stores contacts and calendar items that have no sender or body. Tests author genuine OLE2 containers (tests/support/cfb_writer.py) rather than mocking the format, since a mock of a binary parser is the kind of test that passes while the parser returns nothing. olefile reads but cannot write CFB. LibreOffice is added to the image (Writer and Calc only, not the metapackage). Where the binary is absent the affected types are simply not claimed, so the registry reports "no processor for type" once instead of failing per document. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A soffice process is the heaviest thing the ingest path spawns -- it holds the source document and the rendered output at once -- and nothing capped how many could run. Indexing a folder of .doc files would start one per concurrent task and exhaust the pod's memory, which is precisely what the existing parse-slot limiter exists to prevent for the lighter parse workers. Acquire that same limiter (`document_parse_process_slots`) around the conversion. It is taken and released before the delegated PDF parse acquires it in turn, so the two never nest, and one setting now bounds every heavy document subprocess rather than only half of them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…unit Review follow-up on #1265. The `msg.py` module docstring still claimed the processor was "read with extract-msg" and cited extract-msg's numbers as the reason, left over from before the switch to the olefile reader -- while `_msg_reader.py`'s own docstring told the real story and extract-msg is not even a dependency. It now describes what the module actually does and points at the reader for the rationale, keeping the measured comparison but attributing it correctly. Rename `_libreoffice.convert`'s `timeout` to `timeout_seconds`, matching `document_parse_timeout_seconds` and its siblings, and document why the cap is a parameter at all rather than the caller's own `fail_after`: expiry is translated here into `LibreOfficeError`, the single exception type both callers already handle (python:S7483). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…n size Round-2 review follow-up on #1265. The LibreOffice timeout was hardcoded at 120s with no override, unlike every comparable timeout here (`document_parse_timeout_seconds`, `docling_timeout`, `unstructured_timeout`). Adds `document_office_timeout_seconds` and threads it through registration for both processors that convert. A rendition reached the PDF engine directly rather than back through ProcessorRegistry, so it skipped the size cap an uploaded PDF has to pass. The source is already capped before download, but rendering is not size-preserving -- a modest .doc of dense vector figures can render far larger -- so the cap is now applied to the bytes actually about to be parsed, reusing `oversize_result_for_size` rather than restating the threshold. Also from the review: say why `extract_images=False` is forced for renditions; note in `_msg_reader` that only the plain-text body property is read, and what it would take to add the HTML/RTF variants; cover the legacy `.xls` branch with mocked unit tests so it is not exercised only by the LibreOffice-gated integration test; and cover the code-page decode path and its unknown-CPID fallback, which the `_build_msg` fixture could already produce but no test used. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Round-4 review follow-up on #1265. The setting was explicitly modelled on `document_parse_timeout_seconds`, but only the model got a `Validator(..., gte=1)`. A 0 or negative value therefore reached `anyio.fail_after` in `_libreoffice.convert` and expired every conversion the instant it started -- so the misconfiguration presented as "every .doc/.docx fails to parse", with nothing pointing at the setting that caused it, where its two siblings fail fast at startup and name themselves. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
eecfe1f to
2011233
Compare
|
|
Round-4 finding addressed in
3392 unit tests pass; ruff/ty green; Sonar gate OK. |



Adds three processors, each reading its format the way that measured best
rather than forcing one route on all of them.
Word documents (.doc/.docx) go through a LibreOffice PDF rendition parsed at
the structured tier. Legacy .doc has no working pure-Python reader at all --
markitdown raises UnsupportedFormatException and docling-serve returns
status=failure -- and for .docx the rendition is more faithful than a direct
parse, not less: 98.3% token recall with 22 table rows against mammoth's 17,
because mammoth drops a vertically-merged cell and shifts the rest of that row
one column left, landing answers under the wrong heading. The rendition is a
real PDF, so page numbers and highlight geometry come from the existing path
instead of a second implementation. Hyperlink targets are lost in exchange.
The tier is forced, not classified: the classifier scores a rendition on text
quality and returns tier='fast', and the fast tier extracts ZERO tables from
one, because rendered borders are vector line-art that only find_tables sees.
Spreadsheets (.xls/.xlsx) are read cell-by-cell and deliberately never
rendered. PDF rendering paginated a real workbook into 25 mixed-orientation
pages and recalled only 63.8% of the tokens a direct read recovers -- 567
cells against 2013 -- losing the question numbering entirely, because
LibreOffice honours the print layout. Legacy .xls converts container-to-
container via xlsx, which changes the format without re-laying it out.
Chunks carry
sheet_boundaries(sheet + cell range) where a PDF carriespages; a spreadsheet has no geometry to highlight.
Outlook messages (.msg) are read with a small olefile-based reader rather than
extract-msg, which depends onred-black-tree-mod-- published as an sdistonly, so it cannot satisfy the image's
uv sync --no-build(no dependency'ssetup.py may execute at build time, docker:S8541). The reader is ~80 lines and
recovers slightly more than extract-msg did on a real 1.3 MB thread (15,313
characters against 15,303) about seven times faster, while dropping nine
transitive dependencies. Reading both string variants is the substance of
it: markitdown emits 27 bytes for that message -- the literal "# Email
Message\n\n## Content" -- because it looks for a body variant the message does
not carry, while the same message holds a 14,996-byte body in the other.
Headers are indexed as text, not just metadata: who sent it and when is much
of what an inbox is searched for. Fields are read defensively, since .msg also
stores contacts and calendar items that have no sender or body.
Tests author genuine OLE2 containers (tests/support/cfb_writer.py) rather than
mocking the format, since a mock of a binary parser is the kind of test that
passes while the parser returns nothing. olefile reads but cannot write CFB.
LibreOffice is added to the image (Writer and Calc only, not the metapackage).
Where the binary is absent the affected types are simply not claimed, so the
registry reports "no processor for type" once instead of failing per document.
Stack created with GitHub Stacks CLI • Give Feedback 💬
This PR was generated with the help of AI, and reviewed by a Human
This PR was generated with the help of AI, and reviewed by a Human